Make file filters extensible by turning them into an interface, instead of concrete class.#64
Make file filters extensible by turning them into an interface, instead of concrete class.#64arturaz wants to merge 1 commit intoyasirkula:masterfrom
Conversation
…ad of concrete class.
|
Thank you for the PR. How did you benefit from this change in your own projects? What did your custom IFilter implementations do differently than Filter? |
|
We needed to filter based on regex, something like |
|
Is there any improvement that should be made to get this merged? |
|
I couldn't inspect most of the recent PRs in detail due to my busy schedule for a long time. About this feature, I was originally thinking about adding an event to FileBrowser to filter the files with custom C# code (regex can be applied in that event). That solution seems cleaner to me as I'd like to keep file filters as clean and simple as possible. P.S. Actually I forgot that I've already added that event in a past release: |
yasirkula
left a comment
yasirkula
left a comment
There was a problem hiding this comment.
I've decided to merge this request. Please review my final requests and let me know if you have any questions ^^
| // This is a nonexisting file | ||
| string filename = filenameInput.Substring( startIndex, filenameLength ); | ||
| if( m_pickerMode != PickMode.Folders && filters[filtersDropdown.value].extensions != null ) | ||
| if( m_pickerMode != PickMode.Folders && !filters[filtersDropdown.value].isAllFilesFilter ) |
There was a problem hiding this comment.
We can change it as follows and remove the isAllFilesFilter property: m_pickerMode != PickMode.Folders && filters[filtersDropdown.value] != allFilesFilter
| { | ||
| HashSet<string> extensions = Instance.filters[i].extensionsSet; | ||
| if( extensions != null && extensions.Contains( defaultFilter ) ) | ||
| if( Instance.filters[i].isValidExtension( defaultFilter ) ) |
There was a problem hiding this comment.
We can change it as follows and remove the isValidExtension property: Instance.filters[i].MatchesExtension( defaultFilter, defaultFilter.LastIndexOf( '.' ) != 0 ). Even better, the value of defaultFilter.LastIndexOf( '.' ) != 0 can be cached in a bool defaultFilterHasMultipleSuffixes variable before the for-loop.
| #region Inner Classes | ||
| public class Filter | ||
|
|
||
| public interface IFilter { |
There was a problem hiding this comment.
Please get rid of the free space and put the curly bracket on a new line for consistency (I'm asking this since this is a small commit).
|
We needed to filter based on regex, something like `new Regex(".data-.+?-ourextension")`. This allowed us to implement the custom filter for this case.
…---- On Wed, 13 Apr 2022 19:42:11 +0300 Süleyman Yasir KULA ***@***.***> wrote ----
Thank you for the PR. How did you benefit from this change in your own projects? What did your custom IFilter implementations do differently than Filter?
—
Reply to this email directly, #64 (comment), or https://github.com/notifications/unsubscribe-auth/AAADFA2ELI2LJC3F6ILB3PDVE32OHANCNFSM5TKNPIXA.
You are receiving this because you authored the thread.
|
|
I'm open to merging this pull request but please see my review and also merge the latest commit on the mainstream to your branch. |
2 participants
No description provided.